Only qualify the first symbol in Quick Info #27946

sharwell · 2018-06-18T00:33:59Z

Fixes #547

Ask Mode template not completed

Customer scenario

What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)

Bugs this fixes

(either VSO or GitHub links)

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

Root cause analysis

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

Test documentation updated?

If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.

Fixes dotnet#547

CyrusNajmabadi · 2018-06-18T06:24:08Z

src/EditorFeatures/CSharpTest/QuickInfo/SemanticQuickInfoSourceTests.cs

@@ -745,7 +745,7 @@ public class GenericList<T> { Generic$$List<int> t; }";
        {
            await TestInClassAsync(
 @"event $$EventHandler e;",
-                MainDescription("delegate void System.EventHandler(object sender, System.EventArgs e)"));
+                MainDescription("delegate void System.EventHandler(object sender, EventArgs e)"));


I'm a little concerned about this. Currently, in TypeScript, i often get very minimally-qualified names (basically, i think their quick-info is always 'just the name'). And i'm in a codebase with several duplicate names across modules. Not being qualified is actually a big hindrance because it's not clear what the type actually is...

--

Note: it's a small concern. I think this is worth trying out.

📝 The type is still qualified if the name would not resolve in the current scope.

Echoing @CyrusNajmabadi's concern: this might be a good candidate for a quick design discussion first.

Sure, but remember the new design matches what we already do for all other members. It also goes a long way towards fixing #5 for users who have using declarations in the file. The old behavior is a side effect of the inability to enable context-specific minimization while also including the namespace for the first type.

jasonmalinowski · 2018-06-20T18:18:44Z

src/Compilers/Core/Portable/SymbolDisplay/SymbolDisplayMiscellaneousOptions.cs

@@ -55,5 +55,13 @@ public enum SymbolDisplayMiscellaneousOptions
        /// the special question mark syntax.
        /// </summary>
        ExpandNullable = 1 << 5,
+
+        /// <summary>
+        /// Suppresses minimization of the first visited symbol.


I have to admit, it's not clear to me what "first visited symbol" means in this case as an external consumer of the API. The visitor is an implementation detail of this API, no? It seems the term shouldn't appear here then.

jasonmalinowski · 2018-06-20T18:20:48Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.cs

+
+            return new SymbolDisplayVisitor(
+                this.builder,
+                this.format.WithKindOptions(SymbolDisplayKindOptions.None),


Why the forcing to .None?

The kind is already being printed as part of the nested type. We don't want to include the type kind for the containing type or you could end up with a display like this:

// namespace X namespace X { // class X.A class A { // struct class X.A.B struct B { } } }

jasonmalinowski · 2018-06-20T18:23:19Z

src/EditorFeatures/CSharpTest/QuickInfo/SemanticQuickInfoSourceTests.cs

@@ -745,7 +745,7 @@ public class GenericList<T> { Generic$$List<int> t; }";
        {
            await TestInClassAsync(
 @"event $$EventHandler e;",
-                MainDescription("delegate void System.EventHandler(object sender, System.EventArgs e)"));
+                MainDescription("delegate void System.EventHandler(object sender, EventArgs e)"));


Echoing @CyrusNajmabadi's concern: this might be a good candidate for a quick design discussion first.

Previously, containing types of a nested type were not treated as the "first type" by the symbol display visitor, so generic type constraints were omitted from the displayed result even when SymbolDisplayGenericsOptions.IncludeTypeConstraints was specified in the format. In the new code, containing types of a nested type are still treated as part of the "first type", so the generic constraints are included in the symbol display text.

sharwell · 2018-06-21T12:42:20Z

@dotnet/roslyn-compiler The alternative to commit e282986 appears to be updating the SymbolDisplayGenericsOptions passed to VisualBasicShortErrorMessageFormat and/or VisualBasicErrorMessageFormat to match the values passed to the C# equivalents. It appears the type constraints now appearing in the output were explicitly requested, but ended up dropped from the output due to the way containing types are visited.

cston · 2018-06-21T23:08:31Z

src/Compilers/VisualBasic/Test/Semantic/Semantics/AccessCheck.vb

@@ -1864,10 +1864,10 @@ End Class

            CompilationUtils.AssertTheseDiagnostics(compilation,
 <expected>
-BC30508: 'B' cannot expose type 'A.B(Of T).C' in class 'A' through class 'B'.
+BC30508: 'B' cannot expose type 'A.B(Of T As A.B(Of T).C).C' in class 'A' through class 'B'.


Is this change expected? The resulting error message is harder to read.

Please see my comment above explaining why this happens and a possible alternative. I can go either way but wasn't sure about the broader impact of changing a predefined format.

After discussing this offline, I think we should consider changing the VB diagnostic format to report A(Of T As X).B(Of U As Y) as A(Of T).B(Of U). That change could be handled separately from this PR, although it might block this PR. If you agree, please log a bug, thanks.

cston · 2018-06-21T23:13:36Z

Please add a test to SymbolDisplayTests for C# and VB.

sharwell · 2018-06-22T05:19:52Z

@cston is there a specific code path you'd like to see tested? The changes I made to existing tests seemed to cover most if not all aspects.

cston · 2018-06-22T14:54:24Z

@sharwell, ideally compiler changes should be covered by targeted compiler tests rather than relying on IDE tests. Please add specific tests to SymbolDisplayTests for C# and VB testing ToMinimalDisplayString() and ToDisplayString() with and without the new option. There should be similar existing tests for reference, such as AliasInSpeculativeSemanticModel.

CyrusNajmabadi · 2020-03-01T19:16:27Z

can we potentially move forward with this. The current behavior does seem quite inconsistent and annoying at times (esp. constraints of type-parameters of type-decls).

Only qualify the first symbol in Quick Info

db8d5ca

Fixes dotnet#547

sharwell requested review from a team as code owners June 18, 2018 00:34

jinujoseph added the Area-IDE label Jun 18, 2018

jinujoseph added this to the 15.8 milestone Jun 18, 2018

CyrusNajmabadi reviewed Jun 18, 2018

View reviewed changes

jinujoseph added the Area-Compilers label Jun 19, 2018

jasonmalinowski reviewed Jun 20, 2018

View reviewed changes

sharwell added 2 commits June 20, 2018 16:17

Fix failure to preserve CompilerInternalOptions during transformations

b26f9a3

cston reviewed Jun 21, 2018

View reviewed changes

jinujoseph modified the milestones: 15.8, 16.0 Jun 26, 2018

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jul 13, 2018

jinujoseph modified the milestones: 16.0, 16.3 Jun 9, 2019

sharwell modified the milestones: 16.3, 16.4 Sep 6, 2019

jinujoseph removed this from the 16.4 milestone Dec 11, 2019

sharwell marked this pull request as draft April 8, 2020 22:29

Base automatically changed from master to main March 3, 2021 23:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only qualify the first symbol in Quick Info #27946

Only qualify the first symbol in Quick Info #27946

sharwell commented Jun 18, 2018

CyrusNajmabadi Jun 18, 2018

sharwell Jun 18, 2018

jasonmalinowski Jun 20, 2018

sharwell Jun 21, 2018 •

edited

Loading

jasonmalinowski Jun 20, 2018

jasonmalinowski Jun 20, 2018

sharwell Jun 21, 2018

jasonmalinowski Jun 20, 2018

sharwell commented Jun 21, 2018

cston Jun 21, 2018

sharwell Jun 22, 2018

cston Jun 22, 2018

cston commented Jun 21, 2018

sharwell commented Jun 22, 2018

cston commented Jun 22, 2018 •

edited

Loading

CyrusNajmabadi commented Mar 1, 2020

Only qualify the first symbol in Quick Info #27946

Are you sure you want to change the base?

Only qualify the first symbol in Quick Info #27946

Conversation

sharwell commented Jun 18, 2018

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Jun 21, 2018

sharwell commented Jun 22, 2018

cston commented Jun 22, 2018 • edited Loading

CyrusNajmabadi commented Mar 1, 2020

sharwell Jun 21, 2018 •

edited

Loading

cston commented Jun 22, 2018 •

edited

Loading